Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal for final CodePipeline Actions API shape #459

Merged
merged 1 commit into from
Aug 14, 2018

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Jul 31, 2018

This PR contains what will (hopefully) be the final iteration of the way we structure our CodePipeline Actions.

The API shape is the following:

  1. All Actions integrating with AWS services live in the service's package (so, the CodeCommit Action lives in the aws-codecommit package, the CodeBuild ones - in aws-codebuild, etc.).
  2. There is a separate aws-codepipeline-api package that contains the Actions API, on which the service packages depend on (in order to prevent dependency cycles).
  3. The API of Actions changed - you now provide a Construct as the parent, while the Stage is a required property in the props.

By submitting this pull request, I confirm that my contribution is made under
the terms of the beta license.

@@ -0,0 +1,59 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, there is no support for “re-exporting” symbols in jsii, which means we can’t vend aws-actions to all languages. What I suggest is providing pointers in the codepipeline README to all action types. This will be where people will naturally look for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we kept the aws-codepipeline-aws-actions module in other languages, but remove the re-exporting part? This way, it would leave it as only depending on the other Action modules, and in this way make it a little easier for customers to just import all of them in one go. Does that have enough value?

Also, any comments on the changed names of the Actions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aws-actions feels convoluted and confusing... how about we start without, and see if it's needed?
No comments on the the action names - 👍

@skinny85 skinny85 force-pushed the feature/codepipeline-aws-actions branch from 57515b9 to dd9a394 Compare August 2, 2018 22:45
@skinny85
Copy link
Contributor Author

skinny85 commented Aug 2, 2018

Thanks for the review @eladb . I've submitted a new version, removing the aws-codebuild-aws-actions module completely, and updating the ReadMe of the aws-codepipeline module.

If this looks right to you, feel free to merge this in.

@skinny85
Copy link
Contributor Author

skinny85 commented Aug 3, 2018

Missed updating some outdated comments in the actions.ts file in the aws-codepipeline package in the previous revision, corrected now.

}),
})
```
* [`aws-codecommit-codepipeline`](../aws-codecommit-codepipeline) contains the AWS CodeCommit source Action
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These links are not going to work outside of the repo (i.e. in our docs).
Perhaps you can just put links to the doc website: e.g. https://awslabs.github.io/aws-cdk/refs/_aws-cdk_aws-config.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

```ts
const sourceStage = new Stage(pipeline, 'Source');
```
The most popular Actions for external integrations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use the term "the most popular".
Also, the word "integration", which I know is used internally, is a bit confusing. I would just use something like "Pipeline actions for external services" and then, "Pipeline actions for AWS services"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change.

@skinny85
Copy link
Contributor Author

skinny85 commented Aug 3, 2018

I had one more thought in favor of the aws-codepipeline-aws-actionsmodule (I'm not in love with the name either just to be clear, I'm open to suggestions on a better one) that I wanted your opinion on @eladb .

Currently, we face a question of where to put the external Actions (like GitHub source, Jenkins build, Jenkins test, etc.). Right now, they live in the codepipeline package, but that makes them "special", and different from the AWS Actions. Also, with 100% certainty I can say that not all of the external Actions available in the CodePipeline UI will be in that package (I don't imagine we will put the less popular ones, like Solano CI and Ghost Inspector UI Testing, for example, in there).

We could create a dedicated package for them, something like aws-codepipeline-external-actions (working name). In that case, it seems like having an aws-codepipeline-aws-actions module could be nice, for symmetry and discoverability.

We could even make aws-codepipeline-external-actions be just an empty package that depends on the particular integrations package (like aws-codepipeline-github, aws-codepipeline-jenkins, etc.), to make it more similar with AWS Actions.

I would love to know what's your opinion on this @eladb .

Thanks!

@skinny85 skinny85 force-pushed the feature/codepipeline-aws-actions branch from 21e1c29 to d422f5d Compare August 10, 2018 01:07
@skinny85
Copy link
Contributor Author

skinny85 commented Aug 10, 2018

After our meeting, I've completely re-structured the API according to what we agreed to. I've updated the PR description accordingly.

Sorry for the gigantic CR, but I'd rather rip the band-aid off. Also, it's 95% just moving stuff around (and the necessary changes resulting from that moving around, like different imports because of changing package memberships, etc.).

@@ -0,0 +1 @@
## AWS CodePipeline Actions API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth documenting this module..:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, a miss on my part. Will fix.

};
}

export interface IBucketPermissions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add extensive jsdocs to all these APIs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's strictly an internal API (not customer facing), but I agree some comments are needed.

export interface IStage {
readonly name: string;
readonly pipelineArn: cdk.Arn;
readonly pipelineRole: iam.Role;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of exposing the role, can we expose a function ‘addToRolePolcy’?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot. In the S3 Action, we pass pipelineRole as a parameter into Bucket#grantRead.

}

export interface IStage {
readonly name: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do actions need access to the stage name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used in 2 places right now:

  1. The onStateChange method in Action class (the root of the Actions hierarchy).
  2. The CloudFormationAction abstract class (uses it to name the output artifact).

_addAction(action: Action): void;
}

export interface AbstractActionProps {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be redundant. If you require this prop by the abstract base class it will force everyone to include it in their props. It also has the benefit of have “stage” appear in docs in every actio props. Otherwise, people will have to traverse to the hierarchy.

We can always add this abstraction in the future if we see there’s More to it than just “stage” but this feels like taking it one step too far

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I did it this way is otherwise, we will copy & paste the same documentation for the stage property into 8+ different places. Granted, I didn't write any JsDocs for this property either, but that's a separate discussion ;p.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that's a good enough reason... It's not such a complicated description...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually have a second candidate property for this interface in mind: runOrder.

Does having 2 properties here make this interface valuable enough, in your opinion?

/**
* Construction properties of the {@link AmazonS3Source S3 source action}.
*/
export interface AmazonS3SourceProps extends actions.AbstractActionProps {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn’t that be in S3?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It will be moved as part of #492 once this is shipped (or I'll rebase #492 on top of this, depending on how much back-and-forth we'll have).

readonly name: string;
readonly pipelineArn: cdk.Arn;
readonly pipelineRole: iam.Role;
readonly bucketPermissions: IBucketPermissions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: keep it flat: “grantBucketReadWrite”

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering it will be called on stage, how about: grantPipelineBucketReadWrite?


// export class ECSDeploy extends DeployAction {
// constructor(parent: Stage, name: string, clusterName: string, serviceName: string, fileName?: string) {
// super(parent, name, 'ECS', { minInputs: 1, maxInputs: 1, minOutputs: 0, maxOutputs: 0 }, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we save this code somewhere else (issue?) instead of commented out.. it’s a bit messy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a backlog item in our internal issue tracker for each of these - trust me, I will not forget them :).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to the commented out code. Let's remove it from here. If you want to "save" it somewhere, just paste into your issue.

@@ -29,25 +41,37 @@ export class Stage extends cdk.Construct {
*/
constructor(parent: Pipeline, name: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting the pipeline/stage relationship to also be modeled more idiomatically as a prop
Instead of abusing the construct tree. Let’s align the API completely

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed 100%, but, like I said above, I'd rather do that in a separate PR than make this one even bigger than it already is.

}

/**
* Get a duplicate of this stage's list of actions.
*/
public get actions(): Action[] {
public get actions(): actions.Action[] {
return this._actions.slice();
}

public validate(): string[] {
return this.validateHasActions();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like what I proposed earlier instead of another interface. I would allude to the bucket in the name of the method (because stage.grantReadWrite) is weird

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I proposed above: does stage.grantPipelineBucketReadWrite sound good to you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if you prefer (I hate long names). Since this method is part of the pipeline's stage, I think "pipeline" is not really needed by the context, but that's fine :-)

@skinny85 skinny85 force-pushed the feature/codepipeline-aws-actions branch from d422f5d to 1dcc371 Compare August 10, 2018 18:24
@skinny85
Copy link
Contributor Author

Updated with the feedback from the comments.

@skinny85 skinny85 force-pushed the feature/codepipeline-aws-actions branch from 1dcc371 to dfec209 Compare August 10, 2018 18:44
@skinny85
Copy link
Contributor Author

Missed a VS-Code auto-generated method signature with a union type.

@skinny85
Copy link
Contributor Author

No idea why the build is failing... Stage definitely implements grantPipelineBucketReadWriteTo correctly in TypeScript... could it be because the type in the declaration of grantPipelineBucketReadWriteTo is optional (identity?: iam.IPrincipal)?

@@ -1,24 +0,0 @@
## AWS CodePipline Actions for AWS CloudFormation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a merge issue. Probably needs to be merged into aws-cloudformation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not following this comment 🤔This file has been removed (alongside the entire aws-cloudformation-codepipeline package). This text has been added into the aws-cloudformation README file (here).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops you are right

/**
* Common properties shared by all Actions.
*/
export interface AbstractActionProps {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think this is redundant, but if you insist, I'd call this CommonActionProps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

*
* @param identity the IAM Identity to grant the permissions to
*/
grantPipelineBucketReadWriteTo(identity?: iam.IPrincipal): void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename the To suffix. This is not Objective-C here :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say "rename the To suffix"... do you mean "drop it"? Cause I'm not sure what to rename it to (no pun intended)...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. “Remove” (typo)

}

export interface ActionProps extends AbstractActionProps {
category: ActionCategory;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@eladb
Copy link
Contributor

eladb commented Aug 12, 2018

Regarding the build break. Seems like a jsii bug, but we need to dive deeper. You should be able to run jsii-pacmak --recurse from your package and it will generate and build the java version of your module:

lerna exec jsii-pacmak --scope="@aws-cdk/aws-codepipeline-api" -- --recurse --no-clean -v

--no-clean will preserve the sources (you should see a path somewhere in the output), which will allow you to investigate a bit further.

Let us know what you find out...

@skinny85
Copy link
Contributor Author

This is the JSII-generated code for IStage:

package software.amazon.awscdk.services.codepipeline.api;
/**
 * The abstract interface of a Pipeline Stage that is used by Actions.
 */
public interface IStage extends software.amazon.jsii.JsiiSerializable {
    /**
     * The physical, human-readable name of this Pipeline Stage.
     */
    java.lang.String getName();
    /**
     * The ARN of the Pipeline.
     */
    software.amazon.awscdk.Arn getPipelineArn();
    /**
     * The service Role of the Pipeline.
     */
    software.amazon.awscdk.services.iam.Role getPipelineRole();
    /**
     * Grants read & write permissions to the Pipeline's S3 Bucket to the given Identity.
     * @param identity the IAM Identity to grant the permissions to
     */
    void grantPipelineBucketReadWriteTo(@javax.annotation.Nullable final software.amazon.awscdk.services.iam.IPrincipal identity);
    /**
     * Grants read & write permissions to the Pipeline's S3 Bucket to the given Identity.
     */
    void grantPipelineBucketReadWriteTo();

    /**
     * A proxy class which for javascript object literal which adhere to this interface.
     */
    class Jsii$Proxy extends software.amazon.jsii.JsiiObject implements software.amazon.awscdk.services.codepipeline.api.IStage {
        protected Jsii$Proxy(final software.amazon.jsii.JsiiObject.InitializationMode mode) {
            super(mode);
        }
        /**
         * The physical, human-readable name of this Pipeline Stage.
         */
        public java.lang.String getName() {
            return this.jsiiGet("name", java.lang.String.class);
        }
        /**
         * The ARN of the Pipeline.
         */
        public software.amazon.awscdk.Arn getPipelineArn() {
            return this.jsiiGet("pipelineArn", software.amazon.awscdk.Arn.class);
        }
        /**
         * The service Role of the Pipeline.
         */
        public software.amazon.awscdk.services.iam.Role getPipelineRole() {
            return this.jsiiGet("pipelineRole", software.amazon.awscdk.services.iam.Role.class);
        }
        /**
         * Grants read & write permissions to the Pipeline's S3 Bucket to the given Identity.
         * @param identity the IAM Identity to grant the permissions to
         */
        public void grantPipelineBucketReadWriteTo(@javax.annotation.Nullable final software.amazon.awscdk.services.iam.IPrincipal identity) {
            this.jsiiCall("grantPipelineBucketReadWriteTo", Void.class, java.util.stream.Stream.of(identity).toArray());
        }
    }
}

The interface has 2 variants of the grantPipelineBucketReadWriteTo method - one with an argument, one without, while the Jsii$Proxy only implements the one with the argument.

@skinny85 skinny85 force-pushed the feature/codepipeline-aws-actions branch from dfec209 to 4682338 Compare August 13, 2018 22:18
@skinny85
Copy link
Contributor Author

Updated with the latest comments from @eladb . I won't merge until we figure out the above problem with JSII though.

@skinny85 skinny85 force-pushed the feature/codepipeline-aws-actions branch 2 times, most recently from f33d12c to 015b2fa Compare August 13, 2018 22:40
@skinny85
Copy link
Contributor Author

Missed that an interface rename violated TS lint's "imports must be alphabetized" rule.

@eladb
Copy link
Contributor

eladb commented Aug 14, 2018

@skinny85 you discovered a bug in jsii (aws/jsii#175). Workaround for now is to modify the signature of grantPipelineBucketReadWriteTo such that identity is required and not optional. Open an issue to track fixing this one the jsii bug is fixed please.

BREAKING CHANGE:
1. Introduce a new aws-codepipeline-api package, and move the Actions API there.
2. Move the service-specific Actions back into their service packages.
@skinny85
Copy link
Contributor Author

Updated to make the argument to grantPipelineBucketReadWrite required instead of optional. Created an issue to make it optional again in #567 .

@skinny85 skinny85 merged commit 389e9bc into aws:master Aug 14, 2018
@skinny85 skinny85 deleted the feature/codepipeline-aws-actions branch August 14, 2018 20:06
@eladb
Copy link
Contributor

eladb commented Aug 14, 2018

Hallelujah!

@skinny85
Copy link
Contributor Author

You said it brother 🙇

skinny85 added a commit to skinny85/aws-cdk that referenced this pull request Aug 14, 2018
…instead of parent.

BREAKING CHANGE: this commit changes the way we pass a Pipeline to a newly created Stage.
Instead of passing it as a parent, in the first argument of the constructor,
we now pass it through a separate props object, in the pipeline property.
This is in order to make Stage more consistent with other Constructs,
and with the changes made to the Actions API in aws#459.
rix0rrr added a commit that referenced this pull request Aug 15, 2018
### Features

* __@aws-cdk/cdk__: Tokens can now be transparently embedded into
  strings and encoded into JSON without losing their semantics. This
  makes it possible to treat late-bound (deploy-time) values as if they
  were regular strings ([@rix0rrr] in
  [#518](#518)).
* __@aws-cdk/aws-s3__: add support for bucket notifications to Lambda,
  SNS, and SQS targets ([@eladb] in
  [#201](#201),
  [#560](#560),
  [#561](#561),
  [#564](#564))
* __@aws-cdk/cdk__: non-alphanumeric characters can now be used as
  construct identifiers ([@eladb] in
  [#556](#556))
* __@aws-cdk/aws-iam__: add support for `maxSessionDuration` for Roles
  ([@eladb] in [#545](#545)).

### Changes

* __@aws-cdk/aws-lambda__ (_**BREAKING**_): most classes renamed to be
  shorter and more in line with official service naming (`Lambda`
  renamed to `Function` or ommitted) ([@eladb] in
  [#550](#550))
* __@aws-cdk/aws-codepipeline__ (_**BREAKING**_): move all CodePipeline
  actions from `@aws-cdk/aws-xxx-codepipeline` packages into the regular
  `@aws-cdk/aws-xxx` service packages ([@skinny85] in
  [#459](#459)).
* __@aws-cdk/aws-custom-resources__ (_**BREAKING**_): package was
  removed, and the Custom Resource construct added to the
  __@aws-cdk/aws-cloudformation__ package ([@rix0rrr] in
  [#513](#513))

### Fixes

* __@aws-cdk/aws-lambda__: Lambdas that are triggered by CloudWatch
  Events now show up in the console, and can only be triggered the
  indicated Event Rule. _**BREAKING**_ for middleware writers (as this
  introduces an API change), but transparent to regular consumers
  ([@eladb] in [#558](#558))
* __@aws-cdk/aws-codecommit__: fix a bug where `pollForSourceChanges`
  could not be set to `false` ([@maciejwalkowiak] in
  [#534](#534))
* __aws-cdk__: don't fail if the `~/.aws/credentials` file is missing
  ([@RomainMuller] in
  [#541](#541))
* __@aws-cdk/aws-cloudformation__: fix a bug in the CodePipeline actions
  to correctly support TemplateConfiguration ([@mindstorms6] in
  [#571](#571)).
* __@aws-cdk/aws-cloudformation__: fix a bug in the CodePipeline actions
  to correctly support ParameterOverrides ([@mindstorms6] in
  [#574](#574)).

### Known Issues

* `cdk init` will try to init a `git` repository and fail if no global
  `user.name` and `user.email` have been configured.
skinny85 added a commit to skinny85/aws-cdk that referenced this pull request Aug 15, 2018
…instead of parent.

BREAKING CHANGE: this commit changes the way we pass a Pipeline to a newly created Stage.
Instead of passing it as a parent, in the first argument of the constructor,
we now pass it through a separate props object, in the pipeline property.
This is in order to make Stage more consistent with other Constructs,
and with the changes made to the Actions API in aws#459.
skinny85 added a commit that referenced this pull request Aug 15, 2018
…instead of parent. (#568)

BREAKING CHANGE: this commit changes the way we pass a Pipeline to a newly created Stage.
Instead of passing it as a parent, in the first argument of the constructor,
we now pass it through a separate props object, in the pipeline property.
This is in order to make Stage more consistent with other Constructs,
and with the changes made to the Actions API in #459.
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants